From 0200af138efc9ab3ba5f9095662a719560f28be6 Mon Sep 17 00:00:00 2001 From: "kaf24@firebug.cl.cam.ac.uk" Date: Mon, 21 Nov 2005 14:17:50 +0100 Subject: [PATCH] Fix grant-table transfer implementation. Also fix transfer error paths in network frontend and backend drivers. Signed-off-by: Keir Fraser --- linux-2.6-xen-sparse/arch/xen/kernel/gnttab.c | 31 +++++--- .../drivers/xen/netback/netback.c | 27 +++---- .../drivers/xen/netfront/netfront.c | 32 ++++----- xen/common/grant_table.c | 70 +++++++++---------- 4 files changed, 76 insertions(+), 84 deletions(-) diff --git a/linux-2.6-xen-sparse/arch/xen/kernel/gnttab.c b/linux-2.6-xen-sparse/arch/xen/kernel/gnttab.c index ee038144bc..446e5d359d 100644 --- a/linux-2.6-xen-sparse/arch/xen/kernel/gnttab.c +++ b/linux-2.6-xen-sparse/arch/xen/kernel/gnttab.c @@ -229,20 +229,29 @@ gnttab_grant_foreign_transfer_ref(grant_ref_t ref, domid_t domid) unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref) { - unsigned long frame = 0; + unsigned long frame; u16 flags; - flags = shared[ref].flags; - /* - * If a transfer is committed then wait for the frame address to - * appear. Otherwise invalidate the grant entry against future use. - */ - if (likely(flags != GTF_accept_transfer) || - (synch_cmpxchg(&shared[ref].flags, flags, 0) != - GTF_accept_transfer)) - while (unlikely((frame = shared[ref].frame) == 0)) - cpu_relax(); + * If a transfer is not even yet started, try to reclaim the grant + * reference and return failure (== 0). + */ + while (!((flags = shared[ref].flags) & GTF_transfer_committed)) { + if ( synch_cmpxchg(&shared[ref].flags, flags, 0) == flags ) + return 0; + cpu_relax(); + } + + /* If a transfer is in progress then wait until it is completed. */ + while (!(flags & GTF_transfer_completed)) { + flags = shared[ref].flags; + cpu_relax(); + } + + /* Read the frame number /after/ reading completion status. */ + rmb(); + frame = shared[ref].frame; + BUG_ON(frame == 0); return frame; } diff --git a/linux-2.6-xen-sparse/drivers/xen/netback/netback.c b/linux-2.6-xen-sparse/drivers/xen/netback/netback.c index 1d185e5059..800ce32e36 100644 --- a/linux-2.6-xen-sparse/drivers/xen/netback/netback.c +++ b/linux-2.6-xen-sparse/drivers/xen/netback/netback.c @@ -99,7 +99,6 @@ static unsigned long alloc_mfn(void) return mfn; } -#if 0 static void free_mfn(unsigned long mfn) { unsigned long flags; @@ -120,7 +119,6 @@ static void free_mfn(unsigned long mfn) } spin_unlock_irqrestore(&mfn_lock, flags); } -#endif static inline void maybe_schedule_tx_action(void) { @@ -287,28 +285,19 @@ static void net_rx_action(unsigned long unused) ret = HYPERVISOR_multicall(rx_mcl, mcl - rx_mcl); BUG_ON(ret != 0); + ret = HYPERVISOR_grant_table_op(GNTTABOP_transfer, grant_rx_op, + gop - grant_rx_op); + BUG_ON(ret != 0); + mcl = rx_mcl; - if( HYPERVISOR_grant_table_op(GNTTABOP_transfer, grant_rx_op, - gop - grant_rx_op)) { - /* - * The other side has given us a bad grant ref, or has no - * headroom, or has gone away. Unfortunately the current grant - * table code doesn't inform us which is the case, so not much - * we can do. - */ - DPRINTK("net_rx: transfer to DOM%u failed; dropping (up to) " - "%d packets.\n", - grant_rx_op[0].domid, gop - grant_rx_op); - } gop = grant_rx_op; - while ((skb = __skb_dequeue(&rxq)) != NULL) { netif = netdev_priv(skb->dev); size = skb->tail - skb->data; /* Rederive the machine addresses. */ - new_mfn = mcl[0].args[1] >> PAGE_SHIFT; - old_mfn = 0; /* XXX Fix this so we can free_mfn() on error! */ + new_mfn = mcl->args[1] >> PAGE_SHIFT; + old_mfn = gop->mfn; atomic_set(&(skb_shinfo(skb)->dataref), 1); skb_shinfo(skb)->nr_frags = 0; skb_shinfo(skb)->frag_list = NULL; @@ -321,10 +310,10 @@ static void net_rx_action(unsigned long unused) /* Check the reassignment error code. */ status = NETIF_RSP_OKAY; - if(gop->status != 0) { + if (gop->status != 0) { DPRINTK("Bad status %d from grant transfer to DOM%u\n", gop->status, netif->domid); - /* XXX SMH: should free 'old_mfn' here */ + free_mfn(old_mfn); status = NETIF_RSP_ERROR; } irq = netif->irq; diff --git a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c index 2b6fad55a7..a3efed0d05 100644 --- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c @@ -739,28 +739,23 @@ static int netif_poll(struct net_device *dev, int *pbudget) (i != rp) && (work_done < budget); i++, work_done++) { rx = &np->rx->ring[MASK_NETIF_RX_IDX(i)].resp; + /* - * An error here is very odd. Usually indicates a backend bug, - * low-mem condition, or we didn't have reservation headroom. - */ - if (unlikely(rx->status <= 0)) { - if (net_ratelimit()) - printk(KERN_WARNING "Bad rx buffer " - "(memory squeeze?).\n"); - np->rx->ring[MASK_NETIF_RX_IDX(np->rx->req_prod)]. - req.id = rx->id; - wmb(); - np->rx->req_prod++; + * This definitely indicates a bug, either in this driver or + * in the backend driver. In future this should flag the bad + * situation to the system controller to reboot the backed. + */ + if ((ref = np->grant_rx_ref[rx->id]) == GRANT_INVALID_REF) { + WPRINTK("Bad rx response id %d.\n", rx->id); work_done--; continue; } - ref = np->grant_rx_ref[rx->id]; - - if(ref == GRANT_INVALID_REF) { - printk(KERN_WARNING "Bad rx grant reference %d " - "from dom %d.\n", - ref, np->xbdev->otherend_id); + /* Memory pressure, insufficient buffer headroom, ... */ + if ((mfn = gnttab_end_foreign_transfer_ref(ref)) == 0) { + if (net_ratelimit()) + WPRINTK("Unfulfilled rx req (id=%d, st=%d).\n", + rx->id, rx->status); np->rx->ring[MASK_NETIF_RX_IDX(np->rx->req_prod)]. req.id = rx->id; wmb(); @@ -769,9 +764,8 @@ static int netif_poll(struct net_device *dev, int *pbudget) continue; } - np->grant_rx_ref[rx->id] = GRANT_INVALID_REF; - mfn = gnttab_end_foreign_transfer_ref(ref); gnttab_release_grant_reference(&np->gref_rx_head, ref); + np->grant_rx_ref[rx->id] = GRANT_INVALID_REF; skb = np->rx_skbs[rx->id]; ADD_ID_TO_FREELIST(np->rx_skbs, rx->id); diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index f1b4d345ed..f9f7a3e33b 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -706,35 +706,34 @@ gnttab_transfer( struct pfn_info *page; u32 _d, _nd, x, y; int i; - int result = GNTST_okay; grant_entry_t *sha; + gnttab_transfer_t gop; for ( i = 0; i < count; i++ ) { - gnttab_transfer_t *gop = &uop[i]; + /* Read from caller address space. */ + if ( unlikely(__copy_from_user(&gop, &uop[i], sizeof(gop))) ) + { + /* Caller error: bail immediately. */ + DPRINTK("gnttab_transfer: error reading req %d/%d\n", i, count); + return -EFAULT; + } - page = &frame_table[gop->mfn]; - - if ( unlikely(IS_XEN_HEAP_FRAME(page))) + /* Check the passed page frame for basic validity. */ + page = &frame_table[gop.mfn]; + if ( unlikely(!pfn_valid(gop.mfn) || IS_XEN_HEAP_FRAME(page)) ) { - printk("gnttab_transfer: xen heap frame mfn=%lx\n", - (unsigned long) gop->mfn); - gop->status = GNTST_bad_virt_addr; - continue; - } - - if ( unlikely(!pfn_valid(page_to_pfn(page))) ) - { - printk("gnttab_transfer: invalid pfn for mfn=%lx\n", - (unsigned long) gop->mfn); - gop->status = GNTST_bad_virt_addr; - continue; + /* Caller error: bail immediately. */ + DPRINTK("gnttab_transfer: out-of-range or xen frame %lx\n", + (unsigned long)gop.mfn); + return -EINVAL; } - if ( unlikely((e = find_domain_by_id(gop->domid)) == NULL) ) + /* Find the target domain. */ + if ( unlikely((e = find_domain_by_id(gop.domid)) == NULL) ) { - printk("gnttab_transfer: can't find domain %d\n", gop->domid); - gop->status = GNTST_bad_domain; + DPRINTK("gnttab_transfer: can't find domain %d\n", gop.domid); + (void)__put_user(GNTST_bad_domain, &uop[i].status); continue; } @@ -752,15 +751,16 @@ gnttab_transfer( do { x = y; if (unlikely((x & (PGC_count_mask|PGC_allocated)) != - (1 | PGC_allocated)) || unlikely(_nd != _d)) { - printk("gnttab_transfer: Bad page values %p: ed=%p(%u), sd=%p," + (1 | PGC_allocated)) || unlikely(_nd != _d)) { + /* Caller error: bail immediately. */ + DPRINTK("gnttab_transfer: Bad page %p: ed=%p(%u), sd=%p," " caf=%08x, taf=%" PRtype_info "\n", (void *) page_to_pfn(page), d, d->domain_id, unpickle_domptr(_nd), x, page->u.inuse.type_info); spin_unlock(&d->page_alloc_lock); put_domain(e); - return 0; + return -EINVAL; } __asm__ __volatile__( LOCK_PREFIX "cmpxchg8b %2" @@ -788,16 +788,16 @@ gnttab_transfer( */ if ( unlikely(test_bit(_DOMF_dying, &e->domain_flags)) || unlikely(e->tot_pages >= e->max_pages) || - unlikely(!gnttab_prepare_for_transfer(e, d, gop->ref)) ) + unlikely(!gnttab_prepare_for_transfer(e, d, gop.ref)) ) { DPRINTK("gnttab_transfer: Transferee has no reservation headroom " "(%d,%d) or provided a bad grant ref (%08x) or " "is dying (%lx)\n", - e->tot_pages, e->max_pages, gop->ref, e->domain_flags); + e->tot_pages, e->max_pages, gop.ref, e->domain_flags); spin_unlock(&e->page_alloc_lock); put_domain(e); - gop->status = result = GNTST_general_error; - break; + (void)__put_user(GNTST_general_error, &uop[i].status); + continue; } /* Okay, add the page to 'e'. */ @@ -805,23 +805,23 @@ gnttab_transfer( get_knownalive_domain(e); list_add_tail(&page->list, &e->page_list); page_set_owner(page, e); - + spin_unlock(&e->page_alloc_lock); TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id); - + /* Tell the guest about its new page frame. */ - sha = &e->grant_table->shared[gop->ref]; - sha->frame = gop->mfn; + sha = &e->grant_table->shared[gop.ref]; + sha->frame = gop.mfn; wmb(); sha->flags |= GTF_transfer_completed; - + put_domain(e); - - gop->status = GNTST_okay; + + (void)__put_user(GNTST_okay, &uop[i].status); } - return result; + return 0; } long -- 2.30.2